-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Swik 907 delete deck and transfer ownership #1073
base: master
Are you sure you want to change the base?
Conversation
…ure - first modal and structure.
…hip modal and the missing actions and stuff
TODO is also Kostis comment:
|
I have opened PR slidewiki/deck-service#149 ( not ready yet of course ), and also filled in some missing calls in this branch as well to better test.
For deleting a subdeck, you should use |
What are the possible use cases?
Thus the delete button in edit view for root decks and subdecks opened as own decks. When viewing a subdeck as child of its parent we have the trash icon button for deleting the node. |
… deck edit only on roots and added modal when removing a subdeck as node.
I'm confused with (2) use cases, let me rephrase them as I understand them:
|
… SWIK-907_delete_deck_and_transfer_ownership
If 2.1.1 could have more than one usage entry, then 2.1.3 is valid because usage and parents is the same thing? |
2.1.1 means there's only one usage: the parent currently in view |
…on and related code. Change of precondition to be able to delete/remove a deck
@abijames I updated the branch with the changes we discussed during amsterdam plenary, so in order to delete a subdeck user simply removes it, then they can delete it after loading it from "My decks". So, no option is given whether to simply remove or also delete. I've also moved the delete button to the right so it's more separated from save/cancel buttons. Also, provided a fix for the tranfer ownership dialog not closing sometimes. But I would like @TBoonX to look at that as well, as I commented out some code I didn't know much about. |
} | ||
|
||
unmountTrap() { | ||
this.handleClose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TBoonX I'm not sure about this. Could you review this piece of code ?
I tested this on athena server and have feedback to improve the UI. Functionally it all worked well except for 2 issues:
Suggested UI changes and accessibility issues:
Transfer ownership:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments in thread
Tested on Athena server. Have striked out my comments that have been addressed. |
@abijames regarding the issues you've had:
Your transferring does not affect a fork. If that fork was owned by you, it will remain, unless you transfer ownership of that as well.
This is to be the expected, why is it an issue?
Also correct, transferring ownership removes the deck from the original author's "My Decks". Although I'm not sure where that link is, is it in the search results?
This is intended, yes, so as to allow the new deck owner to review this deck before letting this get published as their own. |
@kprist, thanks for comments. Re (1) my corner is the lack of consistency. Opening the deck from search results to the landing page then the owner of the deck changes. It would be better if the new owner was shown in the search results instead |
I see, if it's the same deck and their metadata change, that is a bug, I think the search indexer indeed fails to update the deck owner. |
@abijames I've resolved most of your comments in the athena deployment. Only thing missing should be
|
@abijames I also verified tranferred owners are properly updated with the search service. There was an issue with some updates in deck service master not in this branch that prevented proper search index updates. Please re-check. Keep in mind that after transferring a deck, it should be removed from search index (as it is also unlisted). The new owner needs to re-publish it it order to be discoverable. |
…n button is now dynamic.
I added a new button to deck edit and a workflow for delete/remove/transfer ownership of a deck. Not all deck routes are working yet.
Please verify that the workflow is correct.
Do you have suggestions for visual improve the user list when transfering ownership?
Missing: